Skip to content

fix: acknowledge oversized dropped events#543

Merged
marandaneto merged 1 commit intoPostHog:mainfrom
marandaneto:fix/oversized-event-flush-hang
Apr 30, 2026
Merged

fix: acknowledge oversized dropped events#543
marandaneto merged 1 commit intoPostHog:mainfrom
marandaneto:fix/oversized-event-flush-hang

Conversation

@marandaneto
Copy link
Copy Markdown
Member

💡 Motivation and Context

Fixes #308.

When an oversized item was removed from the queue and dropped, it was never acknowledged with task_done(). That left Queue.unfinished_tasks non-zero, so flush() could block forever even though the queue was empty.

💚 How did you test it?

  • uv run pytest posthog/test/test_consumer.py::TestConsumer::test_dropping_oversize_msg -q
  • uv run ruff format --check posthog/consumer.py posthog/test/test_consumer.py
  • uv run ruff check posthog/consumer.py posthog/test/test_consumer.py

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran sampo add to generate a changeset file
  • Added the release label to the PR

@marandaneto marandaneto marked this pull request as ready for review April 30, 2026 05:40
@marandaneto marandaneto requested a review from a team as a code owner April 30, 2026 05:40
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Comments Outside Diff (1)

  1. posthog/test/test_consumer.py, line 40-48 (link)

    P2 Prefer parameterised test for oversize drop

    The team prefers parameterised tests. The current test covers only a single oversized item; parameterising over item counts (e.g. 1, 2, 3) would also verify that each of multiple dropped items is individually acknowledged — which is the failure mode this fix addresses.

    @parameterized.expand([
        ("one_oversized_msg", 1),
        ("two_oversized_msgs", 2),
        ("three_oversized_msgs", 3),
    ])
    def test_dropping_oversize_msg(self, _name: str, n: int) -> None:
        q = Queue()
        consumer = Consumer(q, "")
        oversize_msg = {"m": "x" * MAX_MSG_SIZE}
        for _ in range(n):
            q.put(oversize_msg)
        next = consumer.next()
        self.assertEqual(next, [])
        self.assertTrue(q.empty())
        self.assertEqual(q.unfinished_tasks, 0)

    Context Used: Do not attempt to comment on incorrect alphabetica... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/test/test_consumer.py
    Line: 40-48
    
    Comment:
    **Prefer parameterised test for oversize drop**
    
    The team prefers parameterised tests. The current test covers only a single oversized item; parameterising over item counts (e.g. 1, 2, 3) would also verify that each of multiple dropped items is individually acknowledged — which is the failure mode this fix addresses.
    
    ```python
    @parameterized.expand([
        ("one_oversized_msg", 1),
        ("two_oversized_msgs", 2),
        ("three_oversized_msgs", 3),
    ])
    def test_dropping_oversize_msg(self, _name: str, n: int) -> None:
        q = Queue()
        consumer = Consumer(q, "")
        oversize_msg = {"m": "x" * MAX_MSG_SIZE}
        for _ in range(n):
            q.put(oversize_msg)
        next = consumer.next()
        self.assertEqual(next, [])
        self.assertTrue(q.empty())
        self.assertEqual(q.unfinished_tasks, 0)
    ```
    
    **Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
posthog/test/test_consumer.py:40-48
**Prefer parameterised test for oversize drop**

The team prefers parameterised tests. The current test covers only a single oversized item; parameterising over item counts (e.g. 1, 2, 3) would also verify that each of multiple dropped items is individually acknowledged — which is the failure mode this fix addresses.

```python
@parameterized.expand([
    ("one_oversized_msg", 1),
    ("two_oversized_msgs", 2),
    ("three_oversized_msgs", 3),
])
def test_dropping_oversize_msg(self, _name: str, n: int) -> None:
    q = Queue()
    consumer = Consumer(q, "")
    oversize_msg = {"m": "x" * MAX_MSG_SIZE}
    for _ in range(n):
        q.put(oversize_msg)
    next = consumer.next()
    self.assertEqual(next, [])
    self.assertTrue(q.empty())
    self.assertEqual(q.unfinished_tasks, 0)
```

Reviews (1): Last reviewed commit: "fix: acknowledge oversized dropped event..." | Re-trigger Greptile

@marandaneto marandaneto merged commit f4af88a into PostHog:main Apr 30, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flush never completes if too large of item is submitted

2 participants